Sdg23 update user guide installation instructions#404
Sdg23 update user guide installation instructions#404orbeckst merged 11 commits intoMDAnalysis:developfrom
Conversation
lilyminium
left a comment
There was a problem hiding this comment.
Awesome work @namiroues! I've just left some non-blocking recommendations to separate links or maybe use refs to streamline the text a bit.
|
Hi @lilyminium, thanks a lot for the comments, I addressed them and I also refined the instructions to have a clearer version. |
|
Hi @micaela-matta could you please check the changes? |
orbeckst
left a comment
There was a problem hiding this comment.
Please see comments inline, sorry to be brief.
* Fixes #4929 * Removed duplicate installation instructions from API Docs * related to MDAnalysis/UserGuide#404
|
Thanks @orbeckst for your comments. I've updated the installation instructions. I also updated other parts that were outdated. Please have a look. I am sure that you will have some more comments for me :). |
orbeckst
left a comment
There was a problem hiding this comment.
Great rewrite.
I found a few things that occurred to me (and which are generally not that obvious). Thank you for updating this page, it's been high time for an update.
|
Thanks @orbeckst for your comments. I've addressed them. |
orbeckst
left a comment
There was a problem hiding this comment.
Last check before approving: Have you followed the installation instructions and can you confirm that they work?
(I had minor changes but I added them myself from suggestions.)
|
The pre-commit hook check is failing for me. I am just editing on GitHub so I can't easily see where the trailing whitespace is. Can you please run it locally through your pre-commit hook to fix it, @namiroues ? Thanks. |
There was a problem hiding this comment.
I haven't tested the installation instructions myself, but otherwise LGTM! Thanks for all your work @namiroues, looks excellent!
micaela-matta
left a comment
There was a problem hiding this comment.
I second @orbeckst's request to double check installation instructions, but other that that, LGTM! thanks @namiroues
RMeli
left a comment
There was a problem hiding this comment.
Thanks @namiroues, looks very good! I left some additional comments.
4fcc12e to
5ef1085
Compare
|
The RTD build failed because a package had the wrong checksum. I assume that this is an intermittent error (which is out of our control) and it will work the next time you push to this PR. |
There was a problem hiding this comment.
Minor changes inline, the instructions themselves seem good, but not everything works. I kept notes for future follow-up.
Notes from testing on macOS x86_64
❌ mamba Python 3.13.2
I tried in an existing conda/mamba installation. By default it created a Python 3.13.2 environment. MDAnalysis installed fine:
>>> python -c "import MDAnalysis; print(MDAnalysis.__version__)"
2.9.0
However, MDAnalysisTests has issues for me:
mamba install -c conda-forge MDAnalysisTests
gives
Getting conda-forge osx-64
Getting conda-forge noarch
Looking for: ['mdanalysistests', 'python 3.13.2']
509175 packages in https://conda.anaconda.org/conda-forge/osx-64
259522 packages in https://conda.anaconda.org/conda-forge/noarch
Encountered problems while solving.
Problem: nothing provides mdanalysis 0.15.0 needed by mdanalysistests-0.15.0-py27_0
I have no idea why it wants to install 0.15.
Then I try pinning
mamba install -c conda-forge "MDAnalysisTests=2.9.0"
and that fails with
Looking for: ['mdanalysistests 2.9.0.*', 'python 3.13.2']
509175 packages in https://conda.anaconda.org/conda-forge/osx-64
259522 packages in https://conda.anaconda.org/conda-forge/noarch
Encountered problems while solving.
Problem: package mdanalysistests-2.9.0-pyhd8ed1ab_0 requires python >=3.9,<=3.13, but none of the providers can be installed
The mdanalysistests condaforge recipe contains https://github.com/conda-forge/mdanalysistests-feedstock/blob/c9c9a496f8172b2bc9a5c7571092e2ab2f1f8c2f/recipe/meta.yaml#L25
- python >=3.9,<=3.13Based on the package match specification this seems to be the issue because it will not match Python 3.13.2.
I raised conda-forge/mdanalysistests-feedstock#51, which has already been fixed. (So need to test with conda -forge packages ...)
✅ mamba Python 3.12
mamba create -n mdanalysis -c conda-forge mdanalysis python=3.12
mamba activate mdanalysis
mamba install -c conda-forge MDAnalysisTests
Tests in parallel
pytest --disable-pytest-warnings -n 2 -v --pyargs MDAnalysisTests
were a bit wonky for me. Deadlocked once around 87%, second time it happened I started killing python3.12 processes and then suddenly tests continued to run (although possibly only in one runner). Tried again
pytest --disable-pytest-warnings -n 4 -v --pyargs MDAnalysisTests
succeeded (installs 2.9.0 and tests pass).
============ 19714 passed, 363 skipped, 7 xfailed, 2 xpassed, 107840 warnings in 749.94s (0:12:29) ============
✅ , ❌ pip 3.13
Create mamba env with python 3.13
mamba create -n mdapip313 python=3.13 pip
✅ standard
and installed with
pip install --upgrade MDAnalysis
pip install --upgrade MDAnalysisTests
Tests
pytest --disable-pytest-warnings -n 2 -v --pyargs MDAnalysisTests
passed
============ 19014 passed, 781 skipped, 6 xfailed, 2 xpassed, 89945 warnings in 962.55s (0:16:02) =============
❌ Installing extras
pip install --upgrade MDAnalysis[analysis,extra_formats,parallel]
failed because pytng did not build
...
pytng/pytng.c:2398:22: error: implicit declaration of function '_PyList_Extend' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
PyObject* none = _PyList_Extend((PyListObject*)L, v);
^
...
pytng/pytng.c:2436:39: error: implicit declaration of function '_PyInterpreterState_GetConfig' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
__pyx_assertions_enabled_flag = ! _PyInterpreterState_GetConfig(__Pyx_PyThreadState_Current->interp)->optimization_level;
^
pytng/pytng.c:2436:107: error: member reference type 'int' is not a pointer
__pyx_assertions_enabled_flag = ! _PyInterpreterState_GetConfig(__Pyx_PyThreadState_Current->interp)->optimization_level;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
...
pytng/pytng.c:34856:13: error: implicit declaration of function '_PyUnicode_FastCopyCharacters' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
_PyUnicode_FastCopyCharacters(result_uval, char_pos, uval, 0, ulength);
^
...
pytng/pytng.c:38679:70: error: too few arguments to function call, expected 6, have 5
is_little, !is_unsigned);
^
...
pytng/pytng.c:34856:13: error: implicit declaration of function '_PyUnicode_FastCopyCharacters' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
_PyUnicode_FastCopyCharacters(result_uval, char_pos, uval, 0, ulength);
^
...
pytng/pytng.c:38679:70: error: too few arguments to function call, expected 6, have 5
is_little, !is_unsigned);
^
...
#define Py_EXPORTED_SYMBOL __attribute__ ((visibility ("default")))
^
pytng/pytng.c:39895:13: error: implicit declaration of function '_PyGen_SetStopIterationValue' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
_PyGen_SetStopIterationValue(result);
^
...
22 warnings and 9 errors generated.
error: command '/usr/bin/clang' failed with exit code 1
[end of output]
...
Failed to build pytng
ERROR: Failed to build installable wheels for some pyproject.toml based projects (pytng)
orbeckst
left a comment
There was a problem hiding this comment.
Source installation instructions do not work as written.
Please fix/clarify.
|
Thanks @orbeckst. I fixed it and tested it myself. |
orbeckst
left a comment
There was a problem hiding this comment.
Good rewrite, all perfect from my end.
|
Nice work, @namiroues ! |
…#4927) * Fixes MDAnalysis#4929 * Removed duplicate installation instructions from API Docs * related to MDAnalysis/UserGuide#404
This PR updates the installation instructions in the User Guide as part of the SDG documentation cleanup.
This PR is linked to MDAnalysis/mdanalysis#4927
This PR fixes #402 and #405
📚 Documentation preview 📚: https://mdanalysisuserguide--404.org.readthedocs.build/en/404/